Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add DashboardLayout component to @toolpad/core #3554

Merged

Conversation

apedroferreira
Copy link
Member

@apedroferreira apedroferreira commented May 14, 2024

Isolated DashboardLayout component and its docs from #3343 and implemented review feedback.

Docs/demo: https://deploy-preview-3554--mui-toolpad-docs.netlify.app/toolpad/core/components/dashboard-layout/

(once we have a navigation adapter we can make the demo sidebar links work with local state to provide better examples)
(API docs generation to be added next in separate PR)

Summary

Please check the types in packages/toolpad-core/src/AppProvider/AppProvider.tsx for the chosen API.

Possible to import types such as Branding and Navigation from @toolpad/core.
Also added @toolpad/core/themes where the baseTheme we use can be imported from, and that we use as a default theme. Eventually we can provide more themes?

Addressed PR comments

@apedroferreira apedroferreira marked this pull request as ready for review May 14, 2024 17:43
@@ -0,0 +1,69 @@
import * as React from 'react';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Was this hand-written? I think we should port the docs:typescript:formatted script from core

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I hadn't noticed that, I guess I can include it in this PR if it's simple.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've added it here, seems to work fine 9465e74

@@ -0,0 +1,3 @@
# Dashboard Layout API
Copy link
Member

@Janpot Janpot May 15, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the API, let's port the docs generator from X?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can do that in this separate PR, right? #3536

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, the X one, I was checking the Base one, will take a look.

Copy link
Member

@Janpot Janpot May 15, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah yes, base may be better

@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label May 16, 2024
@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label May 17, 2024
@apedroferreira
Copy link
Member Author

This should be ready as a first version of the DashboardLayout - of course we can add and improve a lot in the future.
Will add the generated API docs separately, still finishing work on that but it would be easier to manage if this one is merged.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't believe this file is expected to be here. I think it was generated by the demo formatter script. I added a rule in the ignore list as I saw this happening when porting the script from X repo

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't believe this file is expected here, see my other comment below

export interface NavigationPageItem {
kind?: 'page';
title: string;
path?: string;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With the nomenclature path I'd expect it to be the full path. Maybe we need to come up with another name e.g. like "slug"? That way we can later still add path if we want to add an override in the future.

Suggested change
path?: string;
slug: string;

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤔 The file is already in the project, with different casing

@apedroferreira apedroferreira merged commit 6174b03 into mui:master May 21, 2024
12 checks passed
@apedroferreira apedroferreira deleted the add-dashboard-layout-component branch May 21, 2024 16:29
@apedroferreira apedroferreira restored the add-dashboard-layout-component branch May 24, 2024 15:17
@apedroferreira apedroferreira deleted the add-dashboard-layout-component branch May 24, 2024 15:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs Improvements or additions to the documentation feature: Components Button, input, table, etc. new feature New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants